Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JENKINS-43746 Update step heading to include selected parallel and live duration #1211

Merged

Conversation

NicuPascu
Copy link
Collaborator

@NicuPascu NicuPascu commented Jun 30, 2017

Description

  • If a parallel is selected the step heading should be "Stage / SelectedParallel"
  • Remove duration hover states from indicators
  • Live duration is displayed to right of the step heading

See JENKINS-43746.

Submitter checklist

  • Link to JIRA ticket in description, if appropriate.
  • Change is code complete and matches issue description
  • Appropriate unit or acceptance tests or explanation to why this change has no tests
  • Reviewer's manual test instructions provided in PR description. See Reviewer's first task below.

Reviewer checklist

  • Run the changes and verified the change matches the issue description
  • Reviewed the code
  • Verified that the appropriate tests have been written or valid explanation given

@michaelneale
Copy link
Member

@NicuPascu just checking - this one still on cards? (labels seem set correctly, thanks for that)

@NicuPascu
Copy link
Collaborator Author

@michaelneale yes, is still work in progress because I couldn't make the TimeDuration component to work correctly so now the duration is only visible for a completed step, not while running, took some time off it to give myself some time to think about it, will try to finish it today

@michaelneale
Copy link
Member

@NicuPascu worth picking this one up again some time or has been made long irrelevant? (fine if it has)

@NicuPascu
Copy link
Collaborator Author

@michaelneale I got stuck on adding the live duration and then never gone back to it because of other tickets with higher priorities, I'll update this to master and give it another go

@michaelneale
Copy link
Member

@NicuPascu no worries, just saw the linked ticket. No rush if working on other things, just wanted to close it if wasn't relevant (I missed the ticket link for some reason..)

@cliffmeyers
Copy link
Contributor

There are bugs elsewhere in some of the live duration code, possibly related to the time haromizer stuff. May want to proceed carefully (or not at all) until we can address the harmonizer stuff in a better way. Happy to chat about it more if you're interested.

@i386
Copy link
Contributor

i386 commented Oct 19, 2017

Looks great 👍

One minor change: can you add a space before and after the slash?

Browser Tests/Chrome to Browser Tests / Chrome

screen shot 2017-10-19 at 1 31 45 pm

@NicuPascu
Copy link
Collaborator Author

@i386 Done

@@ -73,7 +73,7 @@ function convertJenkinsNodeDetails(jenkinsNode, isCompleted, skewMillis = 0) {
}
const i18nDuration = timeManager.format(harmonized.durationInMillis, translate('common.date.duration.hint.format', { defaultValue: 'M [month], d [days], h[h], m[m], s[s]' }));

const title = translate(`common.state.${state}`, { 0: i18nDuration });
const title = state !== 'running' ? translate(`common.state.${state}`, { 0: i18nDuration }) : '';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we only want to have the duration as title when it's running? This might be clearer if you reverse the ternary to state == 'running' ? '' : translate(common.state.${state}, { 0: i18nDuration });

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No big deal tho, just took me a little bit to parse. But I'm slow :)

@sophistifunk
Copy link
Collaborator

Code looks OK to me 🐝

IMO the time harmoniser dealie is pointless on durations, because they're scalar, and don't have a timezone.

@NicuPascu NicuPascu merged commit 33c080d into master Oct 26, 2017
@halkeye halkeye deleted the task/JENKINS-43746_update_step_heading_to_include_prallel branch January 24, 2019 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants